LibWeb+LibCrypto: Add Ed448 support in WebCryptoAPI #3009

devgianlu · 2024-12-22T18:34:22Z

Make use of the recently merged OpenSSL PR to implement Ed488 quick and easy.

Apart from implementing Ed448, I've also started defining some macros to aid development of code based on OpenSSL (see OPENSSL_TRY and OPENSSL_TRY_PTR).

The implemention is a bit different from the other curves obviously and now all the methods could be static. I have evaluated some possibilities:

We preallocate common OpenSSL structures when creating the class -> not ideal because then we would need OwnPtrs everywhere
We restructure the class to behave like RSA and require to initialize it with the keys instead of passing them to the methods
We make all methods static and make the class just an utility container

I think the decision can wait based on how the implementation of other things using OpenSSL goes, but wanted to share.

ADKaster · 2024-12-23T17:15:15Z

Libraries/LibCrypto/Curves/Ed448.cpp

+ErrorOr<ByteBuffer> Ed448::generate_private_key()
+{
+    auto* key = OPENSSL_TRY_PTR(EVP_PKEY_Q_keygen(nullptr, nullptr, "ED448"));
+    ArmedScopeGuard const free_key { [&] { EVP_PKEY_free(key); } };


if you never disarm the scope guard, you can use ScopeGuard instead.

ADKaster · 2024-12-23T17:21:03Z

Libraries/LibCrypto/Curves/Ed448.h

+    size_t key_size() { return 57; }
+    size_t signature_size() { return 114; }


these member functions should at least be const.

size_t key_size() const { return 57; }

if not constexpr as well

constexpr size_t key_size() const { return 57; }

ADKaster · 2024-12-23T17:24:56Z

Libraries/LibCrypto/Curves/Ed448.cpp

+ErrorOr<bool> Ed448::verify(ReadonlyBytes public_key, ReadonlyBytes signature, ReadonlyBytes message, ReadonlyBytes context)
+{
+    auto* key = OPENSSL_TRY_PTR(EVP_PKEY_new_raw_public_key_ex(nullptr, "ED448", nullptr, public_key.data(), public_key.size()));
+    ArmedScopeGuard const free_key { [&] { EVP_PKEY_free(key); } };


instead of using all these scope guards, perhaps we should invent some OpenSSL wrappers?

class OpenSSLPrivateKey { public: ErrorOr<OpenSSLPrivateKey> create(StringView name, ReadonlyBytes public_key); ~OpenSSLPrivateKey(); EVP_PKEY* ptr() { return m_ptr; } private: OpenSSLPrivateKey(EVP_PKEY* ptr) : m_ptr(ptr) {}; EVP_PKEY* m_ptr { nullptr }; };

That makes sense, at least for the most common types, I'll protorype something

👍 to me, a one-off scope guard is a handy tool, but using several identical scope guards in every method of a class screams "add some RAII"

Really not sure about TRY(OpenSSL_PKEY::create(EVP_PKEY_Q_keygen, nullptr, nullptr, "ED448"));, does it make any sense?

Yeah that was dumb, should make more sense now

ADKaster · 2024-12-23T17:36:12Z

After this goes in, a comment on WICG/webcrypto-secure-curves#20 seems appropriate

rmg-x · 2024-12-24T00:22:32Z

Libraries/LibCrypto/Curves/Ed448.cpp

+
+namespace Crypto::Curves {
+
+ErrorOr<ByteBuffer> Ed448::generate_private_key()


Non-maintainer take: I think we should avoid fallible returns for things that most likely won't fail. Only because if they do fail, it's either us using the API incorrectly or something completely out of our control like a memory allocation failure. Failing fast in those scenarios seems better than bubbling it up to callers.

Slight bonus is that call sites of these functions become a little less noisy. Anyway, this could just return ByteBuffer and MUST(...) any fallible calls it makes IMO.

Honestly, I am of the opposite idea: since error handling comes at a little cost we should try to cover every case to preserve uniformity and future proofing.

alimpfard · 2024-12-24T15:00:58Z

Libraries/LibCrypto/OpenSSL.h

+            auto* err_message = ERR_error_string(err, nullptr);                              \
+            return Error::from_string_view(StringView { err_message, strlen(err_message) }); \


I don't think this is safe to do, IIRC ERR_error_string will overwrite the static buffer on every call?

You are right, I should make a new buffer beforehand.

Fixed, not sure if there's a better way to make the buffer and pass it around.

ADKaster · 2024-12-24T15:59:04Z

Libraries/LibCrypto/OpenSSL.h

+            VERIFY(err);                                                                                                            \
+            auto err_message = TRY(ByteBuffer::create_zeroed(256));                                                                 \
+            ERR_error_string_n(err, reinterpret_cast<char*>(err_message.data()), 256);                                              \
+            return Error::from_string_view(StringView { err_message.data(), strlen(reinterpret_cast<char*>(err_message.data())) }); \


What you've written is always a use after free. AK::Error cannot own a string. If you want an owned string, you need to use a custom error type in ErrorOr<T, E>

Mhh, I guess we would need something like OpenSSLError which would then escape up to whoever is making the call inhibiting usage of TRY along the way unless ErrorOr<U, OpenSSLError> is used.

I woundn't want to have

ErrorOr<ByteBuffer, OpenSSLError> Ed448::sign(ReadonlyBytes private_key, ReadonlyBytes message, ReadonlyBytes context)

since OpenSSL should be an internal detail. Do you reckon any other way to handle this?

I would just suggest an enum or custom error strings

I have opted for the simplest solution right now: create an error via string literal and log the error. Another option could be to provide an error literal with the macro.

Add a couple of macros to aid error handling with OpenSSL and some RAII classes that manage the lifetime of some OpenSSL objects.

Implement the Ed448 curve for signing and verifying using OpenSSL. The methods could be all made static, but all other curves are not. I think this is material for further refactoring.

Add full support for Ed448 and import relevant tests.

devgianlu requested a review from alimpfard as a code owner December 22, 2024 18:34

ADKaster reviewed Dec 23, 2024

View reviewed changes

devgianlu force-pushed the ed448 branch 3 times, most recently from 1a3dde2 to fc1b5c5 Compare December 23, 2024 20:18

rmg-x reviewed Dec 24, 2024

View reviewed changes

alimpfard reviewed Dec 24, 2024

View reviewed changes

devgianlu force-pushed the ed448 branch from fc1b5c5 to 86ea7f8 Compare December 24, 2024 15:42

ADKaster reviewed Dec 24, 2024

View reviewed changes

devgianlu added 4 commits December 24, 2024 17:57

LibCrypto: Add useful macros and classes for working with OpenSSL

9aa8b31

Add a couple of macros to aid error handling with OpenSSL and some RAII classes that manage the lifetime of some OpenSSL objects.

LibCrypto: Implement Ed448

89dfa98

Implement the Ed448 curve for signing and verifying using OpenSSL. The methods could be all made static, but all other curves are not. I think this is material for further refactoring.

LibCrypto: Add Ed448 OID to know algorithm identifiers

c5a8930

LibWeb: Add Ed448 support in WebCryptoAPI

6d77b02

Add full support for Ed448 and import relevant tests.

devgianlu force-pushed the ed448 branch from 86ea7f8 to 6d77b02 Compare December 24, 2024 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LibWeb+LibCrypto: Add Ed448 support in WebCryptoAPI #3009

LibWeb+LibCrypto: Add Ed448 support in WebCryptoAPI #3009

devgianlu commented Dec 22, 2024

ADKaster Dec 23, 2024

ADKaster Dec 23, 2024

ADKaster Dec 23, 2024

devgianlu Dec 23, 2024

ADKaster Dec 23, 2024

devgianlu Dec 23, 2024

devgianlu Dec 23, 2024 •

edited

Loading

ADKaster commented Dec 23, 2024

rmg-x Dec 24, 2024 •

edited

Loading

devgianlu Dec 24, 2024

alimpfard Dec 24, 2024

devgianlu Dec 24, 2024

devgianlu Dec 24, 2024

ADKaster Dec 24, 2024

devgianlu Dec 24, 2024

alimpfard Dec 24, 2024

devgianlu Dec 24, 2024

		size_t key_size() { return 57; }
		size_t signature_size() { return 114; }


		namespace Crypto::Curves {

		ErrorOr<ByteBuffer> Ed448::generate_private_key()

		auto* err_message = ERR_error_string(err, nullptr); \
		return Error::from_string_view(StringView { err_message, strlen(err_message) }); \

LibWeb+LibCrypto: Add Ed448 support in WebCryptoAPI #3009

Are you sure you want to change the base?

LibWeb+LibCrypto: Add Ed448 support in WebCryptoAPI #3009

Conversation

devgianlu commented Dec 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devgianlu Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

ADKaster commented Dec 23, 2024

rmg-x Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devgianlu Dec 23, 2024 •

edited

Loading

rmg-x Dec 24, 2024 •

edited

Loading